feat: add skill validation CI job - #31

Address Copilot review finding from PR #31: pin the
skill-repo-skill validate workflow to a specific commit SHA
instead of @main for security and reproducibility.

…conversation resolution

Add mechanical checkpoints (GH-30, GH-31) that verify enforce_admins
and required_conversation_resolution via GitHub API, plus an LLM review
checkpoint (GH-32) for the combined audit. Without enforce_admins, admins
can bypass all branch protection rules including unresolved review threads.

Updates verify script to check these settings and security-config.md
reference to document the enforce_admins requirement.

…ecks

GH-30/GH-31 checkpoint commands and the verify script now guard for
gh CLI availability and valid REPO_SLUG before running. When classic
branch protection is not configured, they fall back to querying
GitHub rulesets for equivalent enforcement settings.

…limit

…allowlist

The automated-assessment runner enforces a command allowlist that rejects
command-chaining metacharacters (; && || backticks $()) and only accepts
specific base commands. Four checkpoints failed allowlist validation and
were never actually evaluated against target projects.

GH-6 and GH-23 used `test -f X || test -f Y` chains to test for any of
several files. Rewritten as `type: file_exists` with brace expansion,
which is the runner's first-class idiom for "any of these files".

GH-30 and GH-31 used multi-line YAML literal-block scalars (`target: |`)
to invoke `gh api` for branch protection audits. The runner's simple
line parser sees the literal-block indicator as the first token and
rejects with "'|' not in allowed command whitelist". Even with the
allowlist passing, these cannot be executed mechanically — they require
GitHub API auth context. Converted to `type: gh_api`, which the runner
recognises and skips with a clear evidence string. The semantically
equivalent audit is preserved in GH-32 (llm_reviews).

…dition

Addresses 3 of 10 review threads on PR #77 (Copilot threads on
SKILL.md, checkpoints.yaml line 356, and the enforce_admins
description inside repo-bootstrap.md).

The init script requires the default branch ref to exist (exits 4 on
empty repos). Earlier wording said 'before pushing the first commit'
which is operationally impossible — the script needs a ref to
protect. Corrected uniformly across:

  - SKILL.md 'When to Use' bullet: 'post gh repo create + initial
    push, before first PR' (was 'before first commit/PR')
  - references/repo-bootstrap.md intro paragraph: explicit three-step
    flow (create -> initial push -> protect, then open PRs)
  - checkpoints.yaml GH-31 desc: 'right after gh repo create +
    initial push, before opening the first PR' (was 'immediately
    after gh repo create')

Also corrected repo-bootstrap.md's 'baseline deliberately omits'
section: the template DOES include enforce_admins=false (explicit
permissive default). Only required_signatures is omitted entirely.
Section renamed to 'Deliberately permissive knobs' to match reality.

Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>

Promote the new init-branch-protection.sh to a first-class surface in
the skill:

  - 'When to Use' now leads with 'After gh repo create, before first
    commit/PR' (REQUIRED).
  - New 'Required First Step After gh repo create' section documents
    the two-step flow (apply baseline, then --from-current-checks after
    first CI run), links the snipe-it#17 incident as the pain that
    triggered this, and points at /assess github-project's GH-31
    checkpoint for read-only verification.
  - 'Security & Compliance Quick Checks' gets a one-liner that prints
    whether required_conversation_resolution is enabled on the
    repo's default branch.
  - 'Running Scripts' lists init-branch-protection.sh alongside the
    existing verify-github-project.sh.

The structural enforcement (required_conversation_resolution=true plus
min-1-approver) is what makes the 'abort merge if unresolved threads'
memory rule actually safe. Documentation without a runnable apply path
demonstrably wasn't enough — operator discipline failed in snipe-it#17.

Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>

…ode in GH-31

Severity was already 'error' (verified). This expands the desc to name
the concrete user-visible failure mode that motivated promoting this
to a hard checkpoint in the first place:

  - PR merges may silently include unresolved bot-reviewer feedback
  - Including security findings like token leakage in build logs
  - Concrete incident: netresearch/snipe-it-docker-compose-stack#17

The desc now also points operators at the new init script as the
apply path, so /assess findings have an actionable remediation
embedded in the failure message rather than a generic 'enable it'.

Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>

…dition

Addresses 3 of 10 review threads on PR #77 (Copilot threads on
SKILL.md, checkpoints.yaml line 356, and the enforce_admins
description inside repo-bootstrap.md).

The init script requires the default branch ref to exist (exits 4 on
empty repos). Earlier wording said 'before pushing the first commit'
which is operationally impossible — the script needs a ref to
protect. Corrected uniformly across:

  - SKILL.md 'When to Use' bullet: 'post gh repo create + initial
    push, before first PR' (was 'before first commit/PR')
  - references/repo-bootstrap.md intro paragraph: explicit three-step
    flow (create -> initial push -> protect, then open PRs)
  - checkpoints.yaml GH-31 desc: 'right after gh repo create +
    initial push, before opening the first PR' (was 'immediately
    after gh repo create')

Also corrected repo-bootstrap.md's 'baseline deliberately omits'
section: the template DOES include enforce_admins=false (explicit
permissive default). Only required_signatures is omitted entirely.
Section renamed to 'Deliberately permissive knobs' to match reality.

Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>

…77)

## Summary

Close the structural enforcement gap that just allowed three unresolved
bot-reviewer threads — including a HIGH-severity token leak — to ship to
`main` in
[netresearch/snipe-it-docker-compose-stack#17](netresearch/snipe-it-docker-compose-stack#17).
The skill already documented branch protection and already had a
checkpoint (`GH-31`) for `required_conversation_resolution: true`, but
there was nothing prompting an operator to actually apply protection at
`gh repo create` time, before the first PR opens. By the time `/assess
github-project` would have caught the miss, PRs were already merging.

This PR adds a first-class init surface so the structural rule is
applied at repo-creation time, not audited after the fact.

## What's in the PR

- **`skills/github-project/scripts/init-branch-protection.sh`** —
runnable, idempotent. Takes `<owner>/<repo>`, reads the new template,
PUTs to `repos/{owner}/{repo}/branches/{default_branch}/protection`.
Uses `gh api -X PUT --input -` with JSON body (NOT `--raw-field`, which
silently breaks on nested JSON — that's exactly what wasted three
retries in
[snipe-it#15](netresearch/snipe-it-docker-compose-stack#15)).
Handles empty-repo (exit 4), no access (exit 3), already-compliant (exit
0), and drift on opinionated fields (exit 1 with a per-field diff, no
silent clobber).
- **`skills/github-project/assets/branch-protection.json.template`** —
synthesized from snipe-it's live post-fix state with two deliberate
adjustments: `enforce_admins: false` explicit (so solo-maintainer repos
like ldap-selfservice-… or usercentrics-widgets aren't painted into a
corner), and `required_signatures` *omitted* (not set to `false`) so the
script never resets a repo that has already opted into signing. Both are
documented in the script header as per-repo tightening one-liners.
- **`--from-current-checks` follow-up flag** — after the first CI run
completes on the default branch, re-invoke with this flag and the script
discovers the successful check-run names from the most recent completed
run and PATCHes them in as required status contexts with `strict: true`.
- **SKILL.md** — new \"Required First Step After \`gh repo create\`\"
section right after \"When to Use\". The \"When to Use\" list now leads
with the init step. \"Quick Diagnostics\" gets a one-liner (`gh api
repos/OWNER/REPO/branches/.../protection --jq
'.required_conversation_resolution.enabled'`). \"Running Scripts\" lists
the new script alongside the existing verifier.
- **`checkpoints.yaml` GH-31** — severity was already `error`
(verified). The `desc:` now names the user-visible failure mode (silent
unresolved-thread merges including token leaks like snipe-it#17) and
points operators at the new script as the apply path.
- **README.md** — 7-line callout at the top so someone landing on the
repo page sees the init step without scrolling.

## The new flow

```bash
gh repo create netresearch/new-thing --public --clone
cd new-thing
git push -u origin main          # need at least one commit so the default branch ref exists
bash ../github-project-skill/main/skills/github-project/scripts/init-branch-protection.sh \\
    netresearch/new-thing
# ... open PRs, run CI ...
bash .../init-branch-protection.sh netresearch/new-thing --from-current-checks
```

## What this does NOT fix (deliberately, follow-up)

GitHub branch protection still cannot block on a *requested-but-pending*
review. The snipe-it#17 leak slipped through the **unresolved-threads**
class, which this PR's baseline (`required_conversation_resolution:
true`) DOES close structurally. The **pending-reviewer** class (Copilot
is mid-review, merge anyway) remains a workflow-discipline gap — see
`references/security-config.md` lines 144-152. Closing it requires a
status-check workflow (out of scope here).

## Verification before merge

Run the unresolved-threads GraphQL check before any merge attempt:

```bash
gh api graphql -f query='query{repository(owner:\"netresearch\",name:\"github-project-skill\"){
  pullRequest(number:NUMBER){reviewThreads(first:100){nodes{isResolved}}}
}}' --jq '.data.repository.pullRequest.reviewThreads.nodes | map(select(.isResolved == false)) | length'
```

Abort merge if non-zero. (This PR's own correctness is also a useful
test of the rule it documents.)

## Test plan

- [x] `shellcheck` clean on `init-branch-protection.sh`
- [x] `bash -n` clean
- [x] Live-tested against `netresearch/snipe-it-docker-compose-stack` —
reports `already compliant` (idempotent path works against the repo
whose incident motivated this PR)
- [x] Error paths exercised: no arg → exit 2, bad slug → exit 2, 404
repo → exit 3, bad flag → exit 2
- [x] `markdownlint-cli2` clean on `SKILL.md` and `README.md`
- [x] `yamllint` clean on `checkpoints.yaml`
- [x] All four commits signed (verified via `git log --show-signature`)
- [ ] CI green on this branch
- [ ] Unresolved-threads GraphQL check returns 0 before merge

## Atomic commits

1. `feat(scripts): add init-branch-protection.sh for new-repo bootstrap`
2. `docs(skill): require branch-protection init step before first
PR/commit`
3. `feat(checkpoints): clarify required_conversation_resolution failure
mode in GH-31`
4. `docs(readme): announce required init step at top of README`

…emplate (#79)

## Why

[PR #77](#77)
shipped `assets/branch-protection.json.template` with two deliberate
choices:

- `enforce_admins: false` — solo-maintainer Netresearch repos benefit
from admin-bypass in emergencies
- `required_signatures` omitted — Dependabot/Renovate bot PRs without
per-repo signing setup would otherwise be blocked

`references/security-config.md` was already in the repo with stricter
language:

| Line | Old | Conflict |
|---|---|---|
| ~98 | `enforce_admins` **MUST be `true`** | Template ships `false` |
| ~166 (table) | `required_signatures | true | Enforces GPG/SSH signed
commits` | Template omits the field |

Anyone reading the skill now would see the contradiction as either a
documentation bug or as license to "fix" their per-repo config (and
admin-bypass themselves out of an emergency-merge path).

## What changed

- `enforce_admins` section: switched from "MUST be `true`" to "SHOULD be
`true` on mature multi-maintainer repos as a hardening target". Added
explicit acknowledgement that the init script ships `false` as the
pragmatic baseline, plus the upgrade command + emergency-bypass
rationale.
- `required_signatures` table cell: now shows both states (target:
`true`; init: unset) with the bot-signing precondition and per-repo
upgrade trigger.
- The under-`enforce_admins` security-note callout now points at the
unresolved-threads operator-side safety valve for repos where the
admin-bypass IS the right choice.

## What did NOT change

- No template change. PR #77's `branch-protection.json.template` stays
as-is.
- No script change. The init flow + GH-31 checkpoint behaviour is
unchanged.
- No SKILL.md change. (Already at the 499-word ceiling; left it alone.)

## Test plan

- [x] Markdown lint will run via CI on push
- [x] Word count check: `wc -w skills/github-project/SKILL.md` = 499 (no
change; this PR only touches `security-config.md`)
- [ ] CI green

## Pre-merge gate

I'll run the unresolved-threads GraphQL check before merging — the hard
rule I just had to bake into memory after burning 3 PRs on the same
mistake.

…rotection

The skill taught `gh api repos/OWNER/REPO/branches/main/protection` for its
security quick-check, and GH-31 asserted on that endpoint. It is blind to
rulesets, and the two ways it fails are asymmetric:

  required_status_checks: null              -> reads as "nothing is required"
  required_approving_review_count: 0        -> reads as a real answer

Measured on a repository where both sources are configured: the classic
endpoint reports 0 approvals and no required checks while active rulesets
require 1 approval and 23 status contexts. A compliance document was written
off that reading and attested the repository required neither review nor
checks. Nothing in the response hints at it.

- security-config.md gains an "Effective Branch Rules" section: read both
  sources and take the more restrictive result; bypass actors live on the
  ruleset, not on the branch, and the REST API returns an unresolved numeric
  actor_id.
- GH-31 becomes a `command` checkpoint that passes when EITHER classic
  protection OR a `pull_request` ruleset requires thread resolution. As a
  gh_api check against the classic endpoint it failed a correctly configured
  ruleset-only repository at severity error.
- GH-30's description says its enforce_admins read covers classic protection
  only, and points at the ruleset bypass_actors list.
- The GH-32 llm_review step that checked the same field now checks both.

Two required-check audit recipes found the same way are in the same section: a
required context that is `skipped` on every commit enforces nothing, and
check-run names are prefixed by the calling JOB name, so two jobs on the same
reusable workflow produce two different contexts.

SKILL.md sits at 499 of its 500-word budget on main, so this adds nothing to
it: the quick-check gains the `rules/branches` call and loses the
`bypass_pull_request_allowances` call, which reads the same blind endpoint and
whose replacement is the ruleset bypass_actors list documented in the
reference. 496 words. That budget is also why the material went into the
existing security-config.md rather than a new reference — a new file needs a
new pointer row, and there is no room for one.

Verified: validate-skill.sh reports 0 errors, YAML parses (29 mechanical
checkpoints), GH-31's shell passes `bash -n`, and both of its branches evaluate
against a live repository.

Signed-off-by: Sebastian Mendel <sebastian.mendel@netresearch.de>

…rotection

The skill taught `gh api repos/OWNER/REPO/branches/main/protection` for its
security quick-check, and GH-31 asserted on that endpoint. It is blind to
rulesets, and the two ways it fails are asymmetric:

  required_status_checks: null              -> reads as "nothing is required"
  required_approving_review_count: 0        -> reads as a real answer

Measured on a repository where both sources are configured: the classic
endpoint reports 0 approvals and no required checks while active rulesets
require 1 approval and 23 status contexts. A compliance document was written
off that reading and attested the repository required neither review nor
checks. Nothing in the response hints at it.

- security-config.md gains an "Effective Branch Rules" section: read both
  sources and take the more restrictive result; bypass actors live on the
  ruleset, not on the branch, and the REST API returns an unresolved numeric
  actor_id.
- GH-31 becomes a `command` checkpoint that passes when EITHER classic
  protection OR a `pull_request` ruleset requires thread resolution. As a
  gh_api check against the classic endpoint it failed a correctly configured
  ruleset-only repository at severity error.
- GH-30's description says its enforce_admins read covers classic protection
  only, and points at the ruleset bypass_actors list.
- The GH-32 llm_review step that checked the same field now checks both.

Two required-check audit recipes found the same way are in the same section: a
required context that is `skipped` on every commit enforces nothing, and
check-run names are prefixed by the calling JOB name, so two jobs on the same
reusable workflow produce two different contexts.

SKILL.md sits at 499 of its 500-word budget on main, so this adds nothing to
it: the quick-check gains the `rules/branches` call and loses the
`bypass_pull_request_allowances` call, which reads the same blind endpoint and
whose replacement is the ruleset bypass_actors list documented in the
reference. 496 words. That budget is also why the material went into the
existing security-config.md rather than a new reference — a new file needs a
new pointer row, and there is no room for one.

Verified: validate-skill.sh reports 0 errors, YAML parses (29 mechanical
checkpoints), GH-31's shell passes `bash -n`, and both of its branches evaluate
against a live repository.

Signed-off-by: Sebastian Mendel <sebastian.mendel@netresearch.de>

…rotection (#156)

## The defect

`SKILL.md`'s "Security & Compliance Quick Checks" and checkpoint `GH-31`
both read branch rules from `gh api
repos/OWNER/REPO/branches/main/protection`. That endpoint is blind to
**rulesets**, and its two failure shapes are asymmetric — one looks like
an absence, the other looks like an answer:

| Classic endpoint says | Reads as | Reality on the same repo |
|---|---|---|
| `required_status_checks: null` | nothing is required | 23 required
contexts |
| `required_approving_review_count: 0` | no review required | 1 required
approval |

Measured on `netresearch/t3x-nr-llm`, where classic protection and two
active rulesets coexist. Acting on the classic reading alone, I wrote
into that repository's OpenSSF compliance document that it required
neither an approving review nor any status check. Both statements were
false, and nothing in the API response hints that a second source
exists.

## Changes

**`references/security-config.md` gains an "Effective Branch Rules"
section.** Read both sources; GitHub composes them and applies the most
restrictive result. Bypass actors live on the ruleset, not on the
branch, and the REST API returns `actor_id` as an unresolved number — so
"repository role id 5", not a guessed "admin".

**`GH-31` becomes a `command` checkpoint.** It passes when *either*
classic protection *or* a `pull_request` ruleset requires thread
resolution. As a `gh_api` check against the classic endpoint it fails a
correctly configured ruleset-only repository at severity `error` — a
false negative on the checkpoint that exists to catch unresolved bot
findings.

**`GH-30`** keeps its classic read (`enforce_admins` is genuinely a
classic field) but its description now says so and points at the
ruleset's own `bypass_actors`.

**`GH-32`'s llm_review step 2** checked the same field from the same
blind endpoint; it now checks both.

## Two audit recipes, same origin

- **A required context that is `skipped` on every commit enforces
nothing.** Observed: `fuzz / Fuzz Tests` was required and always
skipped, because the job producing it passed no inputs to its reusable
workflow. The job that actually ran the suite was a different context
and was not required at all.
- **Check-run names are prefixed by the calling JOB name, not the
workflow.** Two jobs on the same reusable produce `fuzz / X` and
`fuzz-mutation / X` — different contexts that look interchangeable. The
section gives the run→jobs loop to measure it instead of inferring.

## A note on the word budget

`SKILL.md` is at **499 of its 500-word limit on `main`**, so the file
cannot accept a net addition — my first push failed `Skill Validation`
at 593 words.

This PR therefore adds nothing to it. The quick-check gains the
`rules/branches` call and loses the `bypass_pull_request_allowances`
call, which reads the same blind endpoint and whose correct replacement
(ruleset `bypass_actors`) is documented in the reference. Net **496
words**.

That budget is also why the material went into the existing
`security-config.md` instead of a new reference file: a new file needs a
new pointer row in the table, and there is no room for one. Worth
knowing as a maintainer — at 499/500 the next contributor hits the same
wall, and the only ways through are trimming existing prose or raising
the cap.

## Verification

```
OK: SKILL.md is 496 words (under 500 limit)
Errors:   0
```

- `validate-skill.sh` (the script CI runs) reports 0 errors locally.
- `yaml.safe_load` parses `checkpoints.yaml`; 29 mechanical checkpoints.
- `GH-31`'s target passes `bash -n`, and both of its branches evaluated
against a live repository (`classic=true ruleset=true`).

The 10 warnings the validator reports (README template sections,
untested scripts, runnable commands in `llm_reviews`) are pre-existing
on `main` and untouched here.

One limit worth stating: I have no ruleset-only repository to
demonstrate the old checkpoint's false negative end to end. The
blindness itself is shown by the table above — the classic endpoint
reporting `0` while a ruleset requires `1` on the same branch.